PEP 748: tlslib - configuration#4958
Conversation
Ease reading the diff of the next commits.
Client-side `trust_store=None` means `TrustStore.system()` but server-side it means "skip client authentication". One could think it means "skip server authentication" when used client-side, so let's not support `None` at all client-side and instead default to `TrustStore.system()`.
|
All commit authors signed the Contributor License Agreement. |
Documentation build overview
709 files changed ·
|
|
cc PEP authors @jvdprng and @woodruffw. |
jvdprng
left a comment
jvdprng
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left two suggestions.
jvdprng
left a comment
jvdprng
left a comment
There was a problem hiding this comment.
On second thought, there is one additional thing we should change.
| lowest_supported_version: TLSVersion | None = None, | ||
| highest_supported_version: TLSVersion | None = None, | ||
| trust_store: TrustStore | None = None, | ||
| trust_store: TrustStore = TrustStore.system(), |
There was a problem hiding this comment.
I remember now why we had None here before: we wanted to avoid creating a shared default object in the API which should be immutable/comparable. So I think we need something else here:
- A sentinel object (we deliberately chose not to do this IIRC)
- Do not define the default, and specify that a missing
TrustStoreshould default to the system trust store. This is my preference.
There was a problem hiding this comment.
Do not define the default, and specify that a missing TrustStore should default to the system trust store. This is my preference.
I don't understand, can you add an example?
There was a problem hiding this comment.
We wanted to avoid creating a shared default object in the API which should be immutable/comparable.
I think there is room for a TrustStore.is_system(truststore) method.
Julien00859
force-pushed
the
Julien00859/tlslib-config
branch
2 times, most recently
from
adfecae to
e14904b
Compare
TLS 1.3 and secure TLS 1.2 both require a certificate and private key server-side. Making the parameter mandatory makes it explicit that one is required. It still is optional client-side.
Julien00859
force-pushed
the
Julien00859/tlslib-config
branch
from
e14904b to
89e8de4
Compare
3 participants
First time contributor 🎉
A few suggestions to make the configuration a bit more explicit. I decided to leave most of the attributes undocumented as they are pretty explicit to me, and to instead only document the few attributes that are different from the client and the server.
PEP 748: ConfigurationError is only for unsupported features
Discussed at trailofbits/tlslib.py#72 (comment)
PEP 748: certificate_chain is mandatory server-side
Discussed at https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543-a-unified-tls-api-for-python/51263/75
PEP 748: disambiguate config trust_store=None
Discussed at https://discuss.python.org/t/pre-pep-discussion-revival-of-pep-543-a-unified-tls-api-for-python/51263/78